Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API redesign: support pipelines #53

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

API redesign: support pipelines #53

wants to merge 50 commits into from

Conversation

coreyti
Copy link
Member

@coreyti coreyti commented Jun 5, 2024

The aim here is to return what we're calling subject(), instead of :ok, for those functions that would not otherwise return some other resource. The result should provide better support for Elixir pipe operations.

@ry4n1m3 ry4n1m3 force-pushed the 20240603-pipeable branch 2 times, most recently from 84ef2c7 to 5be7a4b Compare August 12, 2024 22:12
@ry4n1m3
Copy link
Contributor

ry4n1m3 commented Aug 12, 2024

With this redesign, we should make functions that must return error tuples have a corresponding pipeable version with a! in their name, rather than having a pipeable version that sometimes causes the next step in the pipe to fail having been provided an error tuple.

coreyti and others added 14 commits September 26, 2024 13:42
This change also includes:

- Renaming the `self()` type as `subject()`, to avoid confusion with Elixir's native `self()`.
- Addition of `ChannelOwner.bind!/3`, a helper for calls to `Channel.bind/4`.
- Some minor test tweaks for further clarity.

Co-authored-by: Ryan Spore <[email protected]>
This change also includes:

- Renaming the `self()` type as `subject()`, to avoid confusion with Elixir's native `self()`.
- Addition of `ChannelOwner.bind!/3`, a helper for calls to `Channel.bind/4`.
- Some minor test tweaks for further clarity.
We've switched to the more idiomatic `t()` for function specs.
The `ChannelOwner.post!/3` implementation was problematic in (at least) the following ways:

- It resulted in a somewhat inheritance-like implementation and usage, w/out any need for that;
- It did not handle well differences between "posts" that should result in updates to the "subject" resource vs. retrieval of related resources;
- It did note handle well cases in which the "subject" **should not** be refreshed.

These shortcomings required multiple work-arounds and code verbosity. The introduction of `Channel.post/3` feels much cleaner, more manageable, and idiomatic.
This change provides part 1 of changes in support of pipelining API functions: where functions without the `!` suffix might return `{:error, error}`, those with the `!` will raise a `RuntimeError`.

Note that the underlying implementation is essentially a copy of that found in [`Unsafe`](https://github.com/whitfin/unsafe/tree/main). Minor adjustments have been made to integrate more closely with Playwright, and match desired usage and execution patterns.

An initial usage, for demonstration purposes, may be found at `BrowserContext.add_cookies/2`.
In support of easing the task of locating where to discover or add tests, the filesystem structure for the tests should match that of the implementation.

Note that this change introduced some sort of test pollution. That is resolved via additional cleanup performed `on_exit` as specified in `PlaywrightTest.Case`.
`Playwright.launch/2` now returns a tuple that includes `session`. This, in turn, may be passed to the newly implemented `Playwright.request/1`, which returns an instance of `Playwright.APIRequest`.

`APIRequest` implements `new_context/2` and `new_context!/2`, which create instances of `Playwright.APIRequestContext`.
Really, remove a bogus module, and ensure API modules grouped in API documentation section.
coreyti added 24 commits October 2, 2024 16:53
Including:

- `APIRequestContext.fetch/3`
- `APIResponse.body/1`
- `APIResponse.dispose/1`
- `APIResponse.header/2`
- `APIResponse.json/1`
- `APIResponse.text/1`

Also, removes:

- `APIRequestContext.body/1`. The function has moved to `APIResponse`, where it belongs.
- `APIRequestContext.post/3` for the time being; it needs to be TDD'd and the implementation should simply call `APIRequestContext.fetch/3`.

Incidentally, the placeholder for `Tracing` has been moved to `Playwright.Tracing`.
...where `<method>` is each of:

- `delete/3`
- `get/3`
- `head/3`
- `patch/3`
- `post/3`
- `put/3`

Note that this change depends on an updated `playwright-assets` dependency. v1.44.1 of that library adds HTTP response headers that we need here.
A "WIP" change... still need to implement the (optional) saving and reading of state to/from disk.
Incidentally, also:

- Add more function documentation, including "usage", to `APIResponse`
- Add test of `APIRequestContext.fetch/3` to cover default HTTP method (i.e., "GET")
This fix deserves better test coverage and implementation guards.
However, the function is not yet implemented. A single event is supported in Playwright proper: `disconnected`. And, the event is meant to be emitted from the client-side, upon `Browser.close/1`; we don't yet have a good mechanism for that.

Also, some cleanup.
It's the newly "patched" (co-dependent w/ its related `BrowserContext` that needs to be returned.
Warning:

this function has not yet been successfully tested.
  > So far, the test runs have failed to receive location data and instead
  > experiences "error code 2", which [reportedly](https://developer.mozilla.org/en-US/docs/Web/API/GeolocationPositionError/code)
  > represents `POSITION_UNAVAILABLE` - "The acquisition of the geolocation
  > failed because one or several internal sources of position returned an internal error."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants